fix(core): register sub-composition timelines after async build + lint rule#1638
Conversation
…t rule
When a composition builds its GSAP timeline inside document.fonts.ready (or any
async callback), registering window.__timelines[id] BEFORE the build leaves an
EMPTY timeline registered. The runtime's sub-composition readiness gate treats
"key present" as "ready" and nests the child once — an empty timeline gets
nested empty and is never re-nested, so the frame renders blank when used as a
sub-composition.
- registry/blocks/code-{diff,highlight,morph,scroll,typing}: register the
timeline AFTER the fonts.ready build completes, then call
window.__hfForceTimelineRebind() to re-nest now that it is populated.
- core lint: add rule gsap_timeline_registered_before_async_build to flag the
early-registration anti-pattern, with tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jrusso1020
left a comment
There was a problem hiding this comment.
Small, well-scoped fix + lint guard. Read all 7 files end-to-end. Posting as COMMENT per the team's customer-partner-PR discipline; on merit, ready to stamp.
What I verified
The bug shape is real. Each of the five registry/blocks/code-* blocks has the same pre-fix pattern:
window.__timelines = window.__timelines || {};
var tl = gsap.timeline({ paused: true });
window.__timelines["code-X"] = tl; // synchronous register
document.fonts.ready.then(function () {
tl.from(...); buildEffect(tl, ...); // build happens LATER
});The runtime's sub-composition readiness gate (per the comment) treats "key present in __timelines" as "ready" and nests the child once. Between sync registration and fonts.ready resolution, that "once" nest captures an empty timeline. When fonts.ready later populates the local tl, the nested copy stays empty → blank render when mounted as a sub-composition.
The fix is right and applied byte-equivalently to all 5 blocks: the registration moves to the END of the fonts.ready callback, after all the tweens are added, then calls window.__hfForceTimelineRebind() (guarded by typeof ... === "function") to re-nest the populated timeline. Verified the same 11-line block at the end of each fonts.ready callback in code-diff / code-highlight / code-morph / code-scroll / code-typing.
The lint rule is correctly shaped:
- Finds the first
window.__timelines[registration viacontent.search(/...__timelines\s*\[/). - Finds the first
document.fonts.readyviacontent.search(/document\s*\.\s*fonts\s*\.\s*ready/). - Skips (no finding) when
regIdx >= fontsReadyIdx(registration textually after the boundary = correct pattern). - Confirms the build is actually deferred — checks
tail(the post-fonts.readyslice) for.to(,.from(,.fromTo(, orbuildEffect(before flagging. Prevents false positives on emptyfonts.readycallbacks. stripJsComments(script.content)is called first, so a// document.fonts.readymention in a comment can't fool the offset check.- Severity
errormatches the impact (blank sub-composition renders are a silent UX bug).
Tests cover both the violating and the correct shapes. ✓
Edge cases worth knowing (not blockers)
- Multiple registrations + multiple async boundaries.
content.search()returns FIRST match for each pattern. So a script with both a correct (post-fonts.ready) registration and a separate synchronous registration outside any async boundary would only fire on the first-match ordering. Probably fine for the immediate pattern being fixed (one registration per block) but worth knowing if registries gain multi-timeline blocks later. The current rule is a heuristic, not an AST analysis. - Finding message names the anti-pattern but not the offending key. Useful improvement for future iterations —
findings.push({..., context: { id: <key> }})— but not load-bearing for this PR.
CI
All checks still pending — just opened. No fail conclusions yet. Last-round CodeQL on the sibling Miao PRs is passing; expect the same here.
Stamp posture
Per team discipline on customer-partner PRs, stamp eligibility routes through <@U08E7PV788Z>. From my read this is ready — small, well-tested, clean fix + lint guard. Same hand-back to James as the other three.
Review by Jerrai
miga-heygen
left a comment
There was a problem hiding this comment.
Review — fix(core): register sub-composition timelines after async build + lint rule
Reviewed by Miga
Summary
Clean, well-scoped fix for a real sub-composition race condition. The root cause analysis is solid: window.__timelines[id] = tl registered synchronously before the document.fonts.ready callback populates the timeline, runtime readiness gate sees "key present" → nests once while empty → animation never renders in sub-composition context. The fix (move registration inside the callback, call __hfForceTimelineRebind after) and the lint rule to prevent regression are both well-motivated.
This PR is independent of the #1631/#1632/#1635 series — branches off main, touches only core lint + registry blocks. No overlap with the shared audio engine or script-driven architecture from #1635.
Correctness
The five registry block changes (code-diff, code-highlight, code-morph, code-scroll, code-typing) are structurally identical and correct. The typeof window.__hfForceTimelineRebind === "function" guard is good defensive coding for environments where the runtime hook isn't available.
The lint rule logic is sound:
- Strips JS comments before analysis (via
stripJsComments) — prevents false positives from commented-out code. - Uses string-index comparison (
regIdx < fontsReadyIdx) to detect early registration — simple and effective for this pattern. - The
tailregex confirms actual deferred build (tweens/buildEffect after fonts.ready) so it won't flag harmless cases where fonts.ready is referenced but no build happens there.
Observations (non-blocking)
-
Lint rule scope is narrower than the comment suggests. The leading comment says "or any async callback" but the implementation only checks
document.fonts.ready. That's fine for the five blocks this PR fixes (they all use fonts.ready), but if future blocks defer insidesetTimeout,requestAnimationFrame, orfetch().then(), the rule won't catch them. Worth a// NOTE:in the code acknowledging this is intentionally scoped to fonts.ready for now, so a future author doesn't assume all async boundaries are covered. -
First-match semantics on multiple
__timelines[assignments.content.search()returns the index of the first match. If a script has two assignments — one early (bad) and one inside fonts.ready (good) — the rule correctly flags it because the first match is before fonts.ready. But if someone writes the correct assignment first and then has a redundant early one later in source order (unusual but possible in generated code), the rule would miss it. Unlikely to matter in practice, just flagging the model. -
Test coverage could be slightly broader. The two tests cover the core positive/negative cases well. Consider adding:
- A case with synchronous build (no
document.fonts.readyat all) → should NOT flag. (ThefontsReadyIdx < 0early-return handles this, but a test documents the intent.) - A case where
__timelinesis assigned before fonts.ready but no tweens appear after fonts.ready → should NOT flag. (Thetailregex guard handles this, but again, a test documents it.)
- A case with synchronous build (no
Verdict
Looks good. The fix is correct, well-commented, and the lint rule prevents regression. The observations above are quality-of-life suggestions, none blocking. Recommend approval once CI is green.
Review by Miga
jrusso1020
left a comment
There was a problem hiding this comment.
Self-correction on my round-1: I said "CI still pending — just opened; no fail conclusions yet." That was true at check-time but is stale now — Magi correctly flagged that Test flipped to FAILURE while I was writing. Pulling the failure detail to record it on the PR thread, since both my GH review and Miga's read as "ready once CI green" and that's not the right state.
What's failing
Test check, in bun run --filter '!@hyperframes/producer' test, on @hyperframes/studio. 6 failures, all the same shape:
TypeError: (0 , commitGsapPositionFromDrag) is not a function
at packages/studio/src/hooks/gsapDragCommit.test.ts (lines 81, 100, 120, 168, 222, 393).
Why
Same upstream-squash-merge stacking issue I flagged on #1631 + #1635. This branch carries the renamed source files from those upstream squash-merge commits — gsapDragCommit.ts and the new gsapDragPositionCommit.ts both exist on the branch — but the test file on this branch still imports commitGsapPositionFromDrag from ./gsapDragCommit:
// packages/studio/src/hooks/gsapDragCommit.test.ts (on this branch)
import {
commitGsapPositionFromDrag,
...
} from "./gsapDragCommit"; // <-- gone from this modulegsapDragCommit.ts on the branch no longer exports commitGsapPositionFromDrag (it moved to gsapDragPositionCommit.ts). Net: the import resolves to undefined, the test calls it, TypeError.
#1631 already carries a commit (c312ce70a "test(studio): import commitGsapPositionFromDrag from its actual module") that fixes the test import to point at the new module. So this is a missing-commit-on-branch problem, not a code defect this PR introduces.
Resolution paths
- (a) Land #1631 first. Once #1631 merges to main, this branch rebased onto main picks up the test-import fix, CI goes green, no per-PR work needed here.
- (b) Cherry-pick the test-import fix into this branch if the team wants #1638 unblocked independently. One-commit cherry-pick of
c312ce70a(the test-import fix) would resolve the failure without waiting on #1631 ordering. - (c) Rebase onto current main + cherry-pick if #1631 lands but main moves past it.
Both (a) and (b) leave the code review verdict on #1638 itself unchanged — the sub-composition fix + lint guard are clean. This is purely a stacking-mechanics blocker.
Stamp posture
Unchanged from round-1 (routes through the team), now explicitly: CI gate is RED, do not stamp until one of the resolution paths above clears it.
Review by Jerrai
The function was split out into gsapDragPositionCommit.ts in #1605, but the test kept importing it from ./gsapDragCommit, which no longer exports it — yielding 'is not a function' at runtime. Import from the correct module. Inherited main breakage (same fix as #1631/#1635); fixes the Test CI check on this branch independently of merge order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
Re-review — Round 2
Reviewed by Miga
Diff delta since last review
Compared against my prior review — the diff is unchanged. No new commits, no force-pushes, no code modifications. The two-commit stack is the same:
- The core fix (move
window.__timelines[id] = tlinsidedocument.fonts.ready.then(...)+__hfForceTimelineRebind()call) across all 5 code-* registry blocks. - The test import fix (
commitGsapPositionFromDrag→gsapDragPositionCommit).
CI status
All 47 checks pass. Every regression shard (8 shards), all perf suites, typecheck, lint, format, Windows cross-platform, CodeQL, CLI smoke, runtime contract tests — green across the board. No flakes, no skips on required checks.
Code assessment (unchanged from round 1, confirmed on re-read)
The fix is correct. The race condition is real and well-understood: registering window.__timelines[id] synchronously before the document.fonts.ready callback populates the timeline means the runtime's sub-composition readiness gate sees "key present" → "ready" and nests an empty timeline exactly once. Moving registration after the build completes, plus the __hfForceTimelineRebind() safety net, is the right pattern.
The lint rule is well-designed. gsap_timeline_registered_before_async_build catches the exact pattern — registration index before document.fonts.ready index, with a build-call guard in the tail — and the error message + fixHint are actionable. Comment-stripping via stripJsComments prevents false positives from commented-out code.
Tests cover both polarities. The two new test cases validate the lint rule fires on early registration and stays silent on correct (post-callback) registration.
Prior suggestions status
My round-1 review suggested two additional edge-case tests (multiple <script> tags with split registration/build, and registration inside a nested callback chain). These remain nice-to-haves — the current tests validate the core lint logic correctly, and neither suggested case maps to a pattern that exists in the actual registry blocks today. Not blocking.
Verdict
No new concerns. The fix is correct, the lint rule is solid, CI is fully green. This is ready to merge from a code perspective.
— Review by Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Final stamp after Miguel authorization in Slack.
Reviewed current head 4099b4c0bc: the five registry/blocks/code-* timelines now register only after the document.fonts.ready build completes, then call window.__hfForceTimelineRebind() when available; the new lint rule catches early window.__timelines[id] registration before async timeline construction and has positive/negative tests. The follow-up import fix moves commitGsapPositionFromDrag to ./gsapDragPositionCommit, matching the module export. Live gh pr checks exits 0; GitHub is blocked only on review.
Verdict: APPROVE
Reasoning: The sub-composition readiness race is fixed at each affected registry block, regression coverage is present, and CI is green at the current head.
— Magi
miguel-heygen
left a comment
There was a problem hiding this comment.
Repeat stamp per Miguel authorization in Slack. Current head remains 4099b4c0bc; live gh pr checks exits 0 and GitHub state is clean.
Verdict: APPROVE
Reasoning: Current head is already verified and has no remaining CI or review blockers.
— Magi
What
Fix sub-composition GSAP timelines that build asynchronously, plus a lint rule to catch the anti-pattern. Five registry code blocks register their timeline only after the
document.fonts.readybuild completes; a new core lint rule flags the early-registration pattern.Why
When a composition builds its GSAP timeline inside
document.fonts.ready(glyph metrics must be final before measuring), registeringwindow.__timelines[id]before that async build leaves an empty timeline registered.The runtime's sub-composition readiness gate treats "key present in
__timelines" as "ready" and nests the child once. An empty timeline registered early gets nested empty and is never re-nested — so when the composition is used as a sub-composition, it renders blank (the animation that was added later insidefonts.readyis never picked up).Standalone, the early registration is harmless (the timeline populates in place). The failure only surfaces when the block is mounted as a sub-composition.
How
Registry blocks — register after the build
registry/blocks/code-{diff,highlight,morph,scroll,typing}: move thewindow.__timelines[id] = tlassignment to the end of thefonts.readycallback, after the timeline is fully built, then callwindow.__hfForceTimelineRebind()so the runtime re-nests the now-populated timeline:Core lint — flag the anti-pattern
Add
gsap_timeline_registered_before_async_buildtopackages/core/src/lint/rules/gsap.ts: warns whenwindow.__timelines[...]is assigned before an async build boundary (document.fonts.ready/ async callback), i.e. the registered timeline starts empty. Registration after the async boundary is the correct pattern and is skipped. Tests cover both the violating and the correct shapes.Test plan
bun run test(core) — 647 passed / 5 skipped, including the new rule's tests.Independent of the workflow-skill PRs — branches off
main, touches onlypackages/corelint +registry/blocks.